issue-3781: TFileRingBuffer - operations should return verbose errors - #6866

• 🔴 tasks + storage (build failed, log)
• ✅ blockstore
• ✅ disk + manager
• ✅ filestore

🟢 linux-x86_64-relwithdebinfo target: cloud/disk_manager/ (test time: 268s): all tests PASSED for commit 4ceee30.

🔴 linux-x86_64-relwithdebinfo target: cloud/blockstore/ (test time: 1573s): some tests FAILED for commit 4ceee30.

🔴 linux-x86_64-relwithdebinfo target: cloud/blockstore/ (test time: 918s): some tests FAILED for commit 4ceee30.

🔴 linux-x86_64-relwithdebinfo target: cloud/blockstore/ (test time: 1032s): some tests FAILED for commit 4ceee30.

🔴 linux-x86_64-relwithdebinfo target: cloud/filestore/ (test time: 4770s): some tests FAILED for commit 4ceee30.

🔴 linux-x86_64-relwithdebinfo target: cloud/filestore/ (test time: 4273s): some tests FAILED for commit 4ceee30.

🔴 linux-x86_64-relwithdebinfo target: cloud/filestore/ (test time: 4393s): some tests FAILED for commit 4ceee30.

• 🚫 tasks + storage (cancelled or timed out before reporting completion)
• 🚫 blockstore (cancelled or timed out before reporting completion)
• 🚫 disk + manager (cancelled or timed out before reporting completion)
• 🚫 filestore (cancelled or timed out before reporting completion)

• ✅ tasks + storage
• ✅ blockstore
• ✅ disk + manager
• ✅ filestore

🟢 linux-x86_64-relwithdebinfo target: cloud/tasks/,cloud/storage/ (test time: 267s): all tests PASSED for commit f5419ac.

🟢 linux-x86_64-relwithdebinfo target: cloud/disk_manager/ (test time: 273s): all tests PASSED for commit f5419ac.

🔴 linux-x86_64-relwithdebinfo target: cloud/blockstore/ (test time: 1451s): some tests FAILED for commit f5419ac.

🟢 linux-x86_64-relwithdebinfo target: cloud/blockstore/ (test time: 917s): all tests PASSED for commit f5419ac.

🟢 linux-x86_64-relwithdebinfo target: cloud/filestore/ (test time: 4863s): all tests PASSED for commit f5419ac.

• 🚫 tasks + storage (cancelled or timed out before reporting completion)
• 🚫 blockstore (cancelled or timed out before reporting completion)
• 🚫 disk + manager (cancelled or timed out before reporting completion)
• 🚫 filestore (cancelled or timed out before reporting completion)

AI review:

I pulled the PR (2 commits, 6 files: file_ring_buffer.{h,cpp}, two test files, and the two call sites handle_ops_queue.cpp and persistent_storage.cpp) and reviewed the full diff. Overall the direction is good, but there are several places where the implementation contradicts the new documented contracts, and — ironically — a few places where the new error information is dropped exactly where the PR says it's needed. The PR's stated goal is to make it possible to distinguish success from failure, and corruption from invalid argument, so WriteBackCache and HandleOpsQueue can handle mid-processing corruption — I'll judge it against that.

Bugs / contract violations

1. SetTag violates its own new documentation. The header promises: "Returns an error if the pointer is invalid, the tag value exceeds the value returned by GetMaxTag() or if the buffer is corrupted." But the implementation never checks the tag, and it discards the bool returned by WriteEntryHeader:

auto eh = Data()->ReadEntryHeader(it->second);
eh.Tag = tag;
Data()->WriteEntryHeader(it->second, eh);   // returns false if Tag > MaxTag — ignored
return {};

WriteEntryHeader in file_ring_buffer_format.cpp refuses to write when header.Tag > MaxTag (and MaxTag is 0 for pre-V5 formats), so SetTag(ptr, 100) is a silent no-op reported as success. Compare with Commit, which does check the same return value (Y_ABORT_UNLESS(written)).

I've added an argument check and a test for SetTag

2. Corruption detected mid-operation is still returned as success. This is the exact scenario the PR exists for:

• Visit(): VisitEntries can hit an invalid entry, call SetCorrupted("Invalid entry detected at VisitEntries"), and stop early — but Visit then returns {}. The caller gets S_OK with a silently truncated iteration. It should return an error if IsCorrupted() after VisitEntries (this matters for WriteBackCache recovery, which replays entries via Visit).
• Free(): EraseFreeEntriesFromFront can set the corrupted flag ("Invalid front entry"), yet Free returns {} — and consequently PopFront returns true as well.

Visit and Free should return an error in this case.

3. A failed Commit leaks CurrentAllocation with no way to recover. Commit can now fail via ValidateAccess (e.g., corruption flagged between Alloc and Commit, which is now possible since SetCorrupted is callable from another thread). CurrentAllocation isn't cleared on that path and there is no Discard/Abort API, so every subsequent Alloc/PushBack fails — and, because the CurrentAllocation.HasValue() check in Alloc comes before ValidateAccess, the caller gets a misleading E_INVALID_STATE "Previous allocation is not committed" instead of the corruption error, and the ReportAccessToCorruptedFileRingBuffer critical event never fires from that path.

I've switched checks in Alloc — now it will fail with a corruption error.

4. The error-code taxonomy undercuts the PR's goal. Corruption maps to E_INVALID_STATE, but so does the "Previous allocation is not committed" usage error — a caller trying to distinguish corruption from misuse by code can't. Meanwhile Commit with no pending allocation returns E_ARGUMENT "No allocation to commit", though Commit takes no arguments — that's a state error. I'd pick a consistent mapping (e.g., corruption → one dedicated code, API misuse → another) or callers will end up string-matching messages.

This is not needed. In future, there will be a mandatory argument in Commit.

Call sites still drop the new errors

5. THandleOpsQueue::PopFront discards the result (RequestsToProcess.PopFront();). Combined with fs_impl.cpp::ProcessHandleOpsQueue, a corrupted buffer produces an endless reschedule loop: Empty() is false, Front() returns nullopt, the recovery PopFront() silently does nothing on a corrupted buffer, and the function reschedules — firing ReportHandleOpsQueueProcessError on every tick. The loop shape is pre-existing, but the information needed to break out of it now exists and is thrown away.

This is out of scope from this PR and will be resolved separately #6666

6. THandleOpsQueue::Front counts a corruption error as IncrementParseErrorCount() — conflating precisely the two failure modes this PR separates. A corruption counter (or at least distinct handling) would be more honest.

This is out of scope from this PR and will be resolved separately #6666

7. persistent_storage.cpp: Visit and SetTag discard the returned errors, while Commit/Free convert corruption into Y_ENSURE exceptions. If the graceful handling is planned for a follow-up WriteBackCache PR, fine — but worth saying so explicitly, since until then a corruption detected between Alloc and Commit becomes a throw where it previously couldn't happen.

I've replaced Y_ENSURE with a proper error return.

Thread-safety commit

8. The docs overpromise. "All further operations on the buffer will fail once the flag is set" isn't guaranteed cross-thread: IsCorrupted uses a relaxed load with no synchronization, and an operation already past its ValidateAccess check will complete normally. SetCorrupted from another thread is not a safe "cancel" — I'd soften the wording so WriteBackCache authors don't rely on it. (The atomics themselves are fine; exchange defaults to seq_cst and the dedup of the critical event is correct.)

The wording is:
All further operations on the buffer will fail once the flag is set.
It is ok if currently executing operations will complete successfully.

Tests

• while (rb.PushBack("ABCD").GetResult()) in file_ring_buffer_accessor_ut.cpp (twice) swallows errors — an error is indistinguishable from "buffer full", so the loop exits on a wrong premise. Assert !HasError inside the loop.

Done

• file_ring_buffer_accessor_ut.cpp uses UNIT_ASSERT_VALUES_EQUAL(true, <TResultOrError<bool>>) but the required Out<NCloud::TResultOrError<bool>> specialization is only defined at the bottom of file_ring_buffer_ut.cpp. It links only because both TUs live in the same UNITTEST target — a fragile cross-file dependency (this smells like what broke the first CI build). Move the operator== helpers and Out<> specializations into a shared test header.

Done

• The assertion sweep missed a few calls: rb->PopFront() in the Alloc-while-PopFront test and rb->SetMetadata("abc") in the migration test are still unchecked.

Added asserts

Nits

The variable pushPackResult in handle_ops_queue.cpp should be pushBackResult (appears twice). Header doc grammar is inconsistent ("On success, return true" → "returns"; Free's doc is missing a final period), and the Alloc doc doesn't mention that a pending version migration makes it return nullptr even on an empty buffer — that surprised me reading the code and will surprise callers. Also, PushBack runs ValidateAccess and then Alloc repeats it, which gives corruption precedence over "uncommitted allocation" in PushBack but not in bare Alloc — worth aligning. In the PR description itself: "there methods become thread-safe" → "these", and "TStringBufer" → "TStringBuf".

Unrelated to this PR

On CI: the latest run for f5419ac showed tasks+storage and disk_manager green with blockstore and filestore still in progress at the last bot update, and the previous revision had an actual build failure in tasks+storage plus one failed-build entry in the blockstore and filestore suites — so I'd wait for the remaining two suites before merging.

The most impactful fixes: check WriteEntryHeader's result (and tag range) in SetTag, return an error from Visit/Free when corruption was detected mid-call, and stop discarding PopFront's error in THandleOpsQueue.

• ✅ tasks + storage
• ✅ blockstore
• ✅ disk + manager
• ✅ filestore

🟢 linux-x86_64-relwithdebinfo target: cloud/tasks/,cloud/storage/ (test time: 265s): all tests PASSED for commit 7fa4981.

🟢 linux-x86_64-relwithdebinfo target: cloud/disk_manager/ (test time: 289s): all tests PASSED for commit 7fa4981.

🔴 linux-x86_64-relwithdebinfo target: cloud/blockstore/ (test time: 1443s): some tests FAILED for commit 7fa4981.

🟢 linux-x86_64-relwithdebinfo target: cloud/blockstore/ (test time: 49s): all tests PASSED for commit 7fa4981.

🟢 linux-x86_64-relwithdebinfo target: cloud/filestore/ (test time: 4803s): all tests PASSED for commit 7fa4981.